Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Add DoWithData function #91

Merged
merged 14 commits into from
Aug 4, 2023
Merged

Add DoWithData function #91

merged 14 commits into from
Aug 4, 2023

Conversation

craigpastro
Copy link
Contributor

👋 I am not sure that you will consider this PR, but it is to add a DoWithData generic function that can also return data from a retry, not just an error. That is, it resolves #58. Of course, with this change I had to bump to Go 1.18.

If you think this is a useful feature, I could add some more tests and documentation.

I wasn't sure what to name this function so I borrowed the idea from https://pkg.go.dev/github.com/cenkalti/backoff/v4, which has a RetryWithData function which does a similar thing as this.

Thanks for the very useful library ❤️

@JaSei
Copy link
Collaborator

JaSei commented Apr 21, 2023

Hi @craigpastro This looks good to me. I was thinking about generic and this library, and I have not seen any reason why, but this is good reason. I've just looked at the golang support https://endoflife.date/go and I suppose it's ok to only support Go 1.18+.

Please keep both examples in the documentation.

This was referenced Apr 21, 2023
Closed
Closed
@craigpastro
Copy link
Contributor Author

Hi @JaSei 👋 That's great. It will be nice to get this functionality here. I think you are right about supporting only 1.18+.

I am thinking about what to do with the tests. I could create two of each for Do and DoWithData. Or I could rewrite the majority of them using DoWithData as Do uses DoWithData under the hood. WDYT?

@craigpastro craigpastro marked this pull request as ready for review April 22, 2023 19:42
@craigpastro
Copy link
Contributor Author

craigpastro commented Apr 22, 2023 •
edited
Loading

Hi again @JaSei. I updated the CI to only run with Go versions 1.18 to 1.20 and also to run on pull request. Please let me know if that is okay. Thanks!

Also, would you like to update the examples in ./examples to use DoWithData?

@r-darwish
Copy link

I'm also interested in this. Any chance to get it merged?

@JaSei
Copy link
Collaborator

JaSei commented Aug 3, 2023

I apologize for the long silence. I'm back now. It looks good to me.

@JaSei
Copy link
Collaborator

JaSei commented Aug 3, 2023

I'm just afraid of performance, because generics affects performance. That will be awesome to do benchmark before and with this change 🤔

@JaSei
Copy link
Collaborator

JaSei commented Aug 3, 2023

I've just merged #89 where some benchmark are available

@craigpastro
Copy link
Contributor Author

👋 @JaSei. Welcome back. I merged main into my branch so we can take a look at the benchmarks 👍

@craigpastro
Copy link
Contributor Author

I added similar DoWithData benchmarks. The results are

$ go test -benchmem -run=^$ -bench ^BenchmarkDo github.com/avast/retry-go/v4
goos: darwin
goarch: arm64
pkg: github.com/avast/retry-go/v4
BenchmarkDo-12                                 3         464051111 ns/op            2698 B/op         48 allocs/op
BenchmarkDoWithData-12                         3         433661403 ns/op            2725 B/op         47 allocs/op
BenchmarkDoNoErrors-12                  12527132                96.55 ns/op          208 B/op          4 allocs/op
BenchmarkDoWithDataNoErrors-12          12521830                94.29 ns/op          208 B/op          4 allocs/op
PASS
ok      github.com/avast/retry-go/v4    9.301s

So the generic version is actually faster on my computer 🤷

@JaSei
Copy link
Collaborator

JaSei commented Aug 4, 2023

Do function just wrap DoWithData right? It's not strange it is slower.

@JaSei
Copy link
Collaborator

JaSei commented Aug 4, 2023

current

go test -count 10 -benchmem -run=^$ -bench ^BenchmarkDo github.com/avast/retry-go/v4 | tee current.txt
goos: darwin
goarch: amd64
pkg: github.com/avast/retry-go/v4
cpu: Intel(R) Core(TM) i9-9980HK CPU @ 2.40GHz
BenchmarkDo-16            	       3	 474128987 ns/op	    2730 B/op	      48 allocs/op
BenchmarkDo-16            	       3	 441499631 ns/op	    2725 B/op	      47 allocs/op
BenchmarkDo-16            	       3	 449390845 ns/op	    2693 B/op	      47 allocs/op
BenchmarkDo-16            	       3	 488695333 ns/op	    2725 B/op	      47 allocs/op
BenchmarkDo-16            	       2	 601685067 ns/op	    2704 B/op	      48 allocs/op
BenchmarkDo-16            	       3	 336872997 ns/op	    2693 B/op	      47 allocs/op
BenchmarkDo-16            	       3	 384347911 ns/op	    2725 B/op	      47 allocs/op
BenchmarkDo-16            	       3	 480906307 ns/op	    2693 B/op	      47 allocs/op
BenchmarkDo-16            	       3	 455362447 ns/op	    2693 B/op	      47 allocs/op
BenchmarkDo-16            	       3	 443170384 ns/op	    2693 B/op	      47 allocs/op
BenchmarkDoNoErrors-16    	 6872852	       159.4 ns/op	     208 B/op	       4 allocs/op
BenchmarkDoNoErrors-16    	 7650360	       161.3 ns/op	     208 B/op	       4 allocs/op
BenchmarkDoNoErrors-16    	 7235683	       159.3 ns/op	     208 B/op	       4 allocs/op
BenchmarkDoNoErrors-16    	 7465636	       160.2 ns/op	     208 B/op	       4 allocs/op
BenchmarkDoNoErrors-16    	 7549692	       160.7 ns/op	     208 B/op	       4 allocs/op
BenchmarkDoNoErrors-16    	 7510610	       159.8 ns/op	     208 B/op	       4 allocs/op
BenchmarkDoNoErrors-16    	 7438124	       160.3 ns/op	     208 B/op	       4 allocs/op
BenchmarkDoNoErrors-16    	 7416504	       160.2 ns/op	     208 B/op	       4 allocs/op
BenchmarkDoNoErrors-16    	 7356183	       160.4 ns/op	     208 B/op	       4 allocs/op
BenchmarkDoNoErrors-16    	 7393480	       160.1 ns/op	     208 B/op	       4 allocs/op
PASS
ok  	github.com/avast/retry-go/v4	35.971s

generic

go test -count 10 -benchmem -run=^$ -bench ^BenchmarkDo github.com/avast/retry-go/v4 | tee generic.txt
goos: darwin
goarch: amd64
pkg: github.com/avast/retry-go/v4
cpu: Intel(R) Core(TM) i9-9980HK CPU @ 2.40GHz
BenchmarkDo-16                    	       3	 406306609 ns/op	    2701 B/op	      48 allocs/op
BenchmarkDo-16                    	       3	 419470846 ns/op	    2693 B/op	      47 allocs/op
BenchmarkDo-16                    	       2	 567716303 ns/op	    2696 B/op	      47 allocs/op
BenchmarkDo-16                    	       2	 562713288 ns/op	    2696 B/op	      47 allocs/op
BenchmarkDo-16                    	       3	 418301987 ns/op	    2693 B/op	      47 allocs/op
BenchmarkDo-16                    	       2	 541207332 ns/op	    2696 B/op	      47 allocs/op
BenchmarkDo-16                    	       2	 526211617 ns/op	    2696 B/op	      47 allocs/op
BenchmarkDo-16                    	       2	 517419526 ns/op	    2696 B/op	      47 allocs/op
BenchmarkDo-16                    	       3	 478391497 ns/op	    2693 B/op	      47 allocs/op
BenchmarkDo-16                    	       3	 452548175 ns/op	    2725 B/op	      47 allocs/op
BenchmarkDoWithData-16            	       3	 463040866 ns/op	    2693 B/op	      47 allocs/op
BenchmarkDoWithData-16            	       3	 496158943 ns/op	    2693 B/op	      47 allocs/op
BenchmarkDoWithData-16            	       3	 488367012 ns/op	    2725 B/op	      47 allocs/op
BenchmarkDoWithData-16            	       3	 454618897 ns/op	    2693 B/op	      47 allocs/op
BenchmarkDoWithData-16            	       3	 435430056 ns/op	    2693 B/op	      47 allocs/op
BenchmarkDoWithData-16            	       2	 552289967 ns/op	    2744 B/op	      48 allocs/op
BenchmarkDoWithData-16            	       3	 569748815 ns/op	    2693 B/op	      47 allocs/op
BenchmarkDoWithData-16            	       3	 416597207 ns/op	    2725 B/op	      47 allocs/op
BenchmarkDoWithData-16            	       3	 358455415 ns/op	    2725 B/op	      47 allocs/op
BenchmarkDoWithData-16            	       3	 455297803 ns/op	    2725 B/op	      47 allocs/op
BenchmarkDoNoErrors-16            	 7035135	       161.9 ns/op	     208 B/op	       4 allocs/op
BenchmarkDoNoErrors-16            	 7389806	       161.3 ns/op	     208 B/op	       4 allocs/op
BenchmarkDoNoErrors-16            	 7394016	       161.5 ns/op	     208 B/op	       4 allocs/op
BenchmarkDoNoErrors-16            	 7380039	       162.2 ns/op	     208 B/op	       4 allocs/op
BenchmarkDoNoErrors-16            	 7424865	       162.2 ns/op	     208 B/op	       4 allocs/op
BenchmarkDoNoErrors-16            	 7111860	       160.5 ns/op	     208 B/op	       4 allocs/op
BenchmarkDoNoErrors-16            	 7285305	       162.6 ns/op	     208 B/op	       4 allocs/op
BenchmarkDoNoErrors-16            	 7410627	       160.7 ns/op	     208 B/op	       4 allocs/op
BenchmarkDoNoErrors-16            	 7340961	       161.6 ns/op	     208 B/op	       4 allocs/op
BenchmarkDoNoErrors-16            	 7295727	       164.1 ns/op	     208 B/op	       4 allocs/op
BenchmarkDoWithDataNoErrors-16    	 7357304	       159.9 ns/op	     208 B/op	       4 allocs/op
BenchmarkDoWithDataNoErrors-16    	 6649852	       166.9 ns/op	     208 B/op	       4 allocs/op
BenchmarkDoWithDataNoErrors-16    	 6938404	       176.3 ns/op	     208 B/op	       4 allocs/op
BenchmarkDoWithDataNoErrors-16    	 7181965	       160.4 ns/op	     208 B/op	       4 allocs/op
BenchmarkDoWithDataNoErrors-16    	 7311484	       166.2 ns/op	     208 B/op	       4 allocs/op
BenchmarkDoWithDataNoErrors-16    	 6939157	       169.7 ns/op	     208 B/op	       4 allocs/op
BenchmarkDoWithDataNoErrors-16    	 6648344	       179.0 ns/op	     208 B/op	       4 allocs/op
BenchmarkDoWithDataNoErrors-16    	 6794847	       177.0 ns/op	     208 B/op	       4 allocs/op
BenchmarkDoWithDataNoErrors-16    	 6782588	       171.4 ns/op	     208 B/op	       4 allocs/op
BenchmarkDoWithDataNoErrors-16    	 7279119	       166.9 ns/op	     208 B/op	       4 allocs/op
PASS
ok  	github.com/avast/retry-go/v4	73.128s

benchstat

benchstat current.txt generic.txt
goos: darwin
goarch: amd64
pkg: github.com/avast/retry-go/v4
cpu: Intel(R) Core(TM) i9-9980HK CPU @ 2.40GHz
                      │ current.txt  │             generic.txt             │
                      │    sec/op    │    sec/op     vs base               │
Do-16                   452.4m ± 15%   497.9m ± 16%       ~ (p=0.353 n=10)
DoNoErrors-16           160.2n ±  0%   161.8n ±  1%  +0.97% (p=0.000 n=10)
DoWithData-16                          459.2m ± 20%
DoWithDataNoErrors-16                  168.3n ±  5%
geomean                 269.2µ         280.9µ        +5.42%

                      │ current.txt  │              generic.txt              │
                      │     B/op     │     B/op      vs base                 │
Do-16                   2.635Ki ± 1%   2.633Ki ± 0%       ~ (p=0.745 n=10)
DoNoErrors-16             208.0 ± 0%     208.0 ± 0%       ~ (p=1.000 n=10) ¹
DoWithData-16                          2.646Ki ± 1%
DoWithDataNoErrors-16                    208.0 ± 0%
geomean                   749.2          749.7       -0.05%
¹ all samples are equal

                      │ current.txt │             generic.txt             │
                      │  allocs/op  │ allocs/op   vs base                 │
Do-16                    47.00 ± 2%   47.00 ± 0%       ~ (p=1.000 n=10)
DoNoErrors-16            4.000 ± 0%   4.000 ± 0%       ~ (p=1.000 n=10) ¹
DoWithData-16                         47.00 ± 0%
DoWithDataNoErrors-16                 4.000 ± 0%
geomean                  13.71        13.71       +0.00%
¹ all samples are equal

DoNoErrors is only about 1% slower - that is a good result I thing.

@JaSei
Copy link
Collaborator

JaSei commented Aug 4, 2023

I'm thinking about version.
This is feature request; API is back-forward compatible 🤔 -> 4.4.0
Or would you prefer 5.0.0 because it's relative big change for this module and it will not work with older go than 1.18?

@craigpastro
Copy link
Contributor Author

Do function just wrap DoWithData right? It's not strange it is slower.

Oh. haha. How embarrassing. I obviously wasn't thinking.

I'm thinking about version.
This is feature request; API is back-forward compatible 🤔 -> 4.4.0
Or would you prefer 5.0.0 because it's relative big change for this module and it will not work with older go than 1.18?

That is a good question and those are valid points. In my opinion, with the Go policy of just maintaining the last two releases, bumping the minor version is probably fine.

@JaSei JaSei merged commit b55b1ef into avast:master Aug 4, 2023
11 checks passed
@JaSei
Copy link
Collaborator

JaSei commented Aug 4, 2023

Thanks a lot for the PR @craigpastro. Good job

@craigpastro craigpastro deleted the do-with-data branch August 4, 2023 16:58
@JaSei JaSei mentioned this pull request Aug 4, 2023
Merged
@pzartem
Copy link

pzartem commented Aug 4, 2023

Very nice improvement! Thanks!

kodiakhq bot referenced this pull request in cloudquery/plugin-pb-go Sep 1, 2023
@cq-bot
fix(deps): Update module github.com/avast/retry-go/v4 to v4.5.0 (#99)
5c8cfe1 
This PR contains the following updates:

| Package | Type | Update | Change |
|---|---|---|---|
| [github.com/avast/retry-go/v4](https://github.com/avast/retry-go) | require | minor | `v4.3.4` -> `v4.5.0` |

---

### Release Notes

<details>
<summary>avast/retry-go (github.com/avast/retry-go/v4)</summary>

### [`v4.5.0`](https://github.com/avast/retry-go/releases/tag/v4.5.0)

[Compare Source](https://github.com/avast/retry-go/compare/4.4.0...4.5.0)

#### What's Changed

-   Allow last error to be returned with context error by [@&#8203;willdot](https://github.com/willdot) in [https://github.com/avast/retry-go/pull/96](https://github.com/avast/retry-go/pull/96)

#### New Contributors

-   [@&#8203;willdot](https://github.com/willdot) made their first contribution in [https://github.com/avast/retry-go/pull/96](https://github.com/avast/retry-go/pull/96)

**Full Changelog**: avast/retry-go@4.4.0...v4.5.0

### [`v4.4.0`](https://github.com/avast/retry-go/releases/tag/v4.4.0): (generic support)

[Compare Source](https://github.com/avast/retry-go/compare/4.3.4...4.4.0)

#### What's Changed

-   Go versions by [@&#8203;JaSei](https://github.com/JaSei) in [https://github.com/avast/retry-go/pull/97](https://github.com/avast/retry-go/pull/97)
-   fix: markdown code block format by [@&#8203;mrtc0](https://github.com/mrtc0) in [https://github.com/avast/retry-go/pull/93](https://github.com/avast/retry-go/pull/93)
-   remove error log pre-allocation and add benchmark by [@&#8203;dillonstreator](https://github.com/dillonstreator) in [https://github.com/avast/retry-go/pull/89](https://github.com/avast/retry-go/pull/89)
-   Add DoWithData function by [@&#8203;craigpastro](https://github.com/craigpastro) in [https://github.com/avast/retry-go/pull/91](https://github.com/avast/retry-go/pull/91)

#### New Contributors

-   [@&#8203;mrtc0](https://github.com/mrtc0) made their first contribution in [https://github.com/avast/retry-go/pull/93](https://github.com/avast/retry-go/pull/93)
-   [@&#8203;dillonstreator](https://github.com/dillonstreator) made their first contribution in [https://github.com/avast/retry-go/pull/89](https://github.com/avast/retry-go/pull/89)
-   [@&#8203;craigpastro](https://github.com/craigpastro) made their first contribution in [https://github.com/avast/retry-go/pull/91](https://github.com/avast/retry-go/pull/91)

**Full Changelog**: avast/retry-go@4.3.4...v4.4.0

</details>

---

### Configuration

📅 **Schedule**: Branch creation - "before 4am on the first day of the month" (UTC), Automerge - At any time (no schedule defined).

🚦 **Automerge**: Disabled by config. Please merge this manually once you are satisfied.

♻ **Rebasing**: Whenever PR becomes conflicted, or you tick the rebase/retry checkbox.

🔕 **Ignore**: Close this PR and you won't be reminded about this update again.

---

 - [ ] <!-- rebase-check -->If you want to rebase/retry this PR, check this box

---

This PR has been generated by [Renovate Bot](https://github.com/renovatebot/renovate).
<!--renovate-debug:eyJjcmVhdGVkSW5WZXIiOiIzNi43OC4zIiwidXBkYXRlZEluVmVyIjoiMzYuNzguMyIsInRhcmdldEJyYW5jaCI6Im1haW4ifQ==-->
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Any plans for generic retry from 1.18?
5 participants
@craigpastro @JaSei @r-darwish @pzartem @craigpangea